⚠️ No Changeset found

Latest commit: a3b6cb9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

hds-flight-website – ./

🔍 Inspect: https://vercel.com/hashicorp/hds-flight-website/4QKC51dPu9jDgTvMhmPwMCk4YLwZ
✅ Preview: https://hds-flight-website-git-alert-actions-implementation-hashicorp.vercel.app

hds-components – ./

🔍 Inspect: https://vercel.com/hashicorp/hds-components/8CTDH9SQQkvKPZCSYvW4xV2rp1Qt
✅ Preview: https://hds-components-git-alert-actions-implementation-hashicorp.vercel.app

@amyrlam have a look at the code above. tomorrow we can discuss it together (I'll set up a meeting)

Some thoughts about the proposed patterns

The problem we're trying to solve

According to the design specifications for the Alert component(s), the Button[secondary|tertiary] and the Link(To)Standalone component are allowed (as general guideline) as actions only with the small size:

Also, there is no pre-defined list of actions: in some case we have none, in some cases only a secondary button, in other cases a secondary and a tertiary, in other cases a secondary and a linkstandalone.

So the question (problem) is: how do we allow the consumers to specify which buttons/links to use as actions in the Alert component, and possibly in doing so limit the elements to use only the correct size, while at the same time providing some forms of escape hatch in cases product designers need to "break the rules" for very specific use cases, contexts, exceptions?

Below I try to explain what are the possible options, what are the pros/cons and my thoughts on them.

Option 1 - Just yield in a named block called actions

We could define the Alert component template in this way

<div class="hds-alert" ...attributes>
  [... other code here]
  {{#if (has-block "actions")}}
    <div class="hds-alert__actions">{{yield to="actions"}}</div>
  {{/if}}
</div>

so that the invocation would be like this:

  <Hds::Alert>
    <:actions>
      <Hds::Button @text="My button" @size="small" @color="secondary" />
      <Hds::Link::Standalone @icon="plus" @text="My link" href="#" @size="small" @color="secondary" />
    </:actions>
  </Hds::Alert>

PROs:

• it simple, it works, allows developers to deviate from the standards when needed

CONs:

• allows developers to deviate from the standards, so nothing prevents them from doing errors and using medium sized buttons in normal cases, or primary color for the button
• allows developers to add elements which are normally not supposed to go in the action area of the alert, so unintentional mistakes may happen (they have no indication of what components are supposed to go there, unless they look into the documentation)
• this solution does not allow us any form of control, by default

Option 2 - Yield the actions in the named block as pre-defined components

We could define the Alert component template in this way

<div class="hds-alert" ...attributes>
  [... other code here]
  {{#if (has-block "actions")}}
    <div class="hds-alert__actions">{{yield
          (hash
            Button=(component "hds/button")
            Link::Standalone=(component "hds/link/standalone")
            LinkTo::Standalone=(component "hds/link-to/standalone")
          )
    to="actions"}}</div>
  {{/if}}
</div>

so that the invocation would be like this:

  <Hds::Alert>
     <:actions as |A|>
      <A:Button @text="My button" @size="small" @color="secondary" />
      <A:Link::Standalone @icon="plus" @text="My link" href="#" @size="small" @color="secondary" />
    </:actions>
  </Hds::Alert>

PROs:

• It's more clear which components are expected in the named block (I suspect this may work with autocomplete in Ember) and which are not, while leaving anyway an escape hatch (it will work also with elements that are not in the pre-defined hashed list)

CONs:

• We still ask engineers to know/guess that only the small size and the secondardy/tertiary colors are allowed.

Questions:

• Is there maybe a way to pass certain pre-defined props to the components when we declare them in the hash? Something like this: (hash Button=(component "hds/button" @size="small"))?

Option 3 - Use an array of objects to declare the actions

My understanding is that this is not a standard way to do things in Ember (more of a common pattern in React) but maybe would make sense to adopt it anyway? Imagine being able to declare an "action" via a list of key/value pairs (each one of them could be also validated and throw an assertion if a user tries to use the wrong color/size, for example).

Let's start from the invocation:

<Hds::Alert
  @actions={{array
    (hash element="button" text="My button" color="secondary")
    (hash element="link" icon="plus" text="My link" color="secondary" href="#")
  }}
/>

This would mean the component template would look like this:

<div class="hds-alert" ...attributes>
  {{#if @actions}}
    <div class="hds-alert__actions">
      {{#each @actions as |action|}}
        {{#if (eq action.element "button")}}
          <Hds::Button
            @icon={{action.icon}}
            @iconPosition={{action.iconPosition}}
            @text={{action.text}}
            @color={{action.color}}
            @size="small"
          />
        {{/if}}
        {{#if (eq action.element "link-to")}}
          <Hds::LinkTo::Standalone
            @icon={{action.icon}}
            @iconPosition={{action.iconPosition}}
            @text={{action.text}}
            @color={{action.color}}
            @route="components.alert"
            @size="small"
          />
        {{/if}}
        {{#if (eq action.element "link")}}
          <Hds::Link::Standalone
            @icon={{action.icon}}
            @iconPosition={{action.iconPosition}}
            @text={{action.text}}
            @color={{action.color}}
            @href="#"
            @size="small"
          />
        {{/if}}
      {{/each}}
    </div>
  {{/if}}
</div>

PROs:

• We have 100% control on what size/color actions are allowed and what are not
• We could provide anyway an escape hatch for the users using a named block

CONs:

• It's likely an anti-pattern in Ember?
• Would it work for events, attributes, other things we need to pass to the elements too?

Conclusion

At the moment we have a slight preference for option number 2, seems the most "idiomatic" to Ember, but we would like to hear your thoughts on this. We may see more and more problems like this, and would be nice to have shared, agreed patterns to solve them in the HDS components codebase.

✅ I'd like to cast a vote in preference of "Option 1 - Just yield in a named block called actions" from the perspective of an HDS consumer. 👍 Let's keep it simple, composable, and stay flexible.

In terms of the cited CONS: listed for your "Option 1":

• "allows developers to deviate from the standards ..."
• "allows developers to add elements ..."
• "this solution does not allow us any form of control, by default"

Looking at this holistically, my inference is that these seem motivated by a desire to try to govern designs. Is that correct? I think that's absolutely an important consideration for consistency, and encouraging best practices.

However, I'd argue that it'd be much more effective to catch heuristic design misses during design review, rather than through a restrictive Ember component API in code during implementation. I've observed UI engineers generally implement whatever's in Figma mocks, very faithfully (often to the pixel) IRL.

Given that the Figma mock is being faithfully implemented: IMO setting up UI engineers to "police" design inconsistencies after a design has been approved and handed off sounds like it could cause friction, and be potentially wasteful.

(🧂 Grain of salt ... these are my own opinions / observations.)

I vote for Option 1 with Option 2 being a close second (nevermind I take that back).

Also, I am curious about Option 3. It looks like Option 3 limits the order that the actions render. Is this intentional?

I'm in favour of option 1 or option 2, which use yielded blocks. Either of these are idiomatic of Ember, whereas option 3 relies on an invented schema (therefor greater cognitive overhead) for passing in "actions", a term that is already used by Ember components.

I would caution against exerting compositional control as noted in option 3. Over time, designs can and will specify new and novel compositions that design systems didn't anticipate. It seems unnecessarily fraught to attempt to enumerate all possible use cases up front. Such novel compositions could be both intentional and useful. Thus, we should leave ourselves as much flexibility as possible to achieve novel designs.

I see the argument for consistency. However, design is rarely 100% consistent in the real world. Let's plan for that.

Just gonna chime in here and repeat what others said: FE more often than not builds what design specs out, so if things are consistent across designs, there shouldn't be a worry about what gets built. That mixed with the likelihood of users copy and pasting from docs (which I think is very likely) I think the instances of "mis-use" are likely to be small.

I'd think that option 1 is still possible if option 2 is implemented (there's nothing forcing consumers to use the yielded components). So either of these is fine for me.

@didoo your question about passing default args when using the component helper - you can pass args, but not HTML attributes, and I believe the syntax is without the @ - (hash Button=(component "hds/button" size="small")).

"I'd think that option 1 is still possible if option 2 is implemented (there's nothing forcing consumers to use the yielded components). So either of these is fine for me."

💡 Ah, that's a great observation @meirish ... the lowest-common-denominator of "Option 2" is really "Option 1" in practice anyway.

Thank you @jesdavpet @venusang @randallmorey @meirish for the precious feedback. I am closing this PR for now, and I'll implement option 2 in a separate PR so the history is clean (and we avoid the conflicts).

/cc @amyrlam

Update after #245

While finalizing #245 we came around an issue related to whitespace, in particular on how Ember treats whitespace and named blocks and yielded/contextual components. I'll try to explain the issue here, what we learned trying to solve it, and how this affected the API implementation of Alert the component. The reason for adding all these learnings here is for future reference.

The problem

The initial implementation of the Alert component saw this code/API:

When tested locally, in the showcase page, everything worked perfectly. But when trying to adopt the component in Cloud UI (see https://github.com/hashicorp/cloud-ui/pull/2429) we came across a problem with the whitespace.

Essentially the invocation in Cloud UI looked like this:

When this alert was rendered, it had an extra visible space below the text because the actions block was rendered even if no contextual component was yielded to the actions block:

The reason for this is in the if declared inside the <:actions> named block invocation:

<:actions as |A|>
  {{#if (some condition)}}
    [some content]
  {{/if}}
</:actions>

At first sight this code makes totally sense, and one would not expect to see the block generated in the component (remember the condition {{#if (has-block 'actions')}}). In reality, Ember considers the whitespace before and after the IF inside the <:actions> block as content, so the check {{#if (has-block 'actions')}} returns true.

The first thought was to just invert the code in the invocation:

{{#if (some condition)}}
  <:actions as |A|>
    [some content]
  </:actions>
{{/if}}

and move the {{if}} check outside the <:actions> block, but in that case Ember throws an error:

The reason for this, even though counter-intuitive, is that the <:actions> is not a tag but an argument passed to the function/class of the component (see ember-polyfills/ember-named-blocks-polyfill#25).

After spending almost one day trying to figure out this, I asked help in the #tech-frontend Slack channel and I found out that there was a way in Ember to remove the extra whitespace using a specific syntax in Handlebars (~ or "squishies"): https://handlebarsjs.com/guide/expressions.html#whitespace-control

So I added these squishies in the component code, and this fixed the issue. Yay!!

But...

The "Toast" component

In the continuation of the work around #245 we have decided to split the Alert component, and have a dedicated Toast component only for that. The intention was to simply re-use the Alert(Inline) component, inside the Toast component (which at this point is nothing more than a simple wrapper around the Alert.

After a few back and forth and tests and request for help (again) in the Slack channel we came up with this nice API for the Toast component (this would avoid re-defining all the internals of Alert also inside the Toast):

But again the whitespace problem hit us! The space before/after the yield, inside the <:actions> was considered "content" and the "actions" block was rendered again, non empty! 😭 This time was even worse: for some reasons, even if we added the ~ squishies everywhere the block was rendered anyway.

The reason for that (that's my understanding) is that what we're doing somehow is passing a "function" not just a block, or content, so the block is not undefined anyway. We tested also an approach following what is done in this UI library code (https://github.com/prysmex/ember-eui) essentially defining a specific prop ignoreAlertBlock and pass it down to the Alert component, and at first sight seemed to work, but then when the Toast was invoked with an {{#if}} inside the whitespace problem appeared again.

The only way, again, to solve the problem was not only to remove the whitespace from the Toast

but (much worse!) we would have to ask to do the same for the consumers! Which is not feasible, it would be a really bad DX.

In the thread on Slack it was pointed out that this is a known issue, and no one has a real solution to the problem:

• Conditional blocks emberjs/rfcs#735 (comment)
  • Conditional blocks emberjs/rfcs#735 (comment)
• Yieldable named blocks emberjs/rfcs#460 (comment)

The "solution"

At the end the decision I took was this:

• use CSS to add the space to the actions children instead of the block itself (not great, but it works)
• remove the named blocks from the Alert component: they were causing too many issues, and in this way the API of the Alert component would follow the same pattern used for the Dropdown (direct declaration of yielded components)
  • with this it was also possible to remove all the ~ squishies from the code (the whitespace was not a problem anymore at this point, because there were no named blocks carrying it forward in their invocations.

The final API for the component looked like this at the end (we also added a generic container, as yielded component):

  <Hds::Alert @type="inline" @title="Title here" @description="Description here" as |A|>
    <A.Button @text="Your action" @color="secondary" />
    <A.LinkTo::Standalone @color="secondary" @icon="plus" @text="Another action" @route="index" />
    <A.Link::Standalone @icon="arrow-right" @iconPosition="trailing" @text="Another action" href="#" />
    <A.Generic>[some content here]</A.Generic>
  </Hds::Alert>

Update:

Even more on the whitespace problem:

• see description here: Add "Link::Inline" component (05) #231
• see repro test here: https://ember-twiddle.com/3575bcb7229f8c689339408eb0dcbed2